Call module_close() after freeing the string name to avoid a segfault#33
Call module_close() after freeing the string name to avoid a segfault#33tyler274 wants to merge 1 commit into
Conversation
In the existing order there is a valgrind confirmed segfault when we free `record->filename` after the module was closed. Signed-off-by: tyler <tyler274port@gmail.com>
|
I tested this change after packaging the library for NixOS with a direnv enabled build environment. Bumped the version over there but can cherry pick the changes into this branch for 0.3.3. Ill try to pull this into gssproxy to see if the C version with valgrind still reports a segfault after I wake up. |
Will do. This manifested and is mentioned here gssapi/gssproxy#14 The segfault goes away when the function is disabled during configuration. |
|
@simo5 As requested: If I leave the function enabled at all in gssproxy it errors. |
|
Reproducer is taking a bit longer, as the script I have expects the reproducible build environment I set up in the gssproxy for NixOS. |
|
Here is my kernel hardening configuration in the meantime: https://github.com/tyler274/nixos/blob/main/modules/nixos/hardening.nix |
|
Reproducer script here. Drop it in a fresh checkout of gssproxy and it should be able to setup the build environment and reproduce. #!/usr/bin/env bash
#
# Reproducer for the gssproxy verto_cleanup double-free.
#
# When gssproxy is linked against krb5's embedded (BUILTIN_MODULE) libverto,
# calling verto_cleanup() on exit attempts to free:
# - a static struct ("builtin_record" in libverto.so)
# - a string literal (the empty module-name baked into the .so read-only segment)
# Neither is heap memory, so the free() aborts the daemon on every clean
# shutdown ("free(): invalid pointer").
#
# This script builds the C gssproxy from this checkout and runs it under
# valgrind, then sends SIGTERM to trigger the exit path. Everything it needs
# (a pinned nixpkgs, valgrind, and an inline gssproxy build) is fetched/defined
# by Nix here, so it is fully self-contained: it does NOT use the repo's nix/
# packaging and runs anywhere Nix is installed -- NixOS, Fedora, Debian, etc.
# The only prerequisite is a working `nix` (single- or multi-user); no channels,
# NIX_PATH, or flake configuration are required.
#
# Usage (from anywhere):
# ./reproduce-verto-cleanup.sh
#
# Expected output: two "Invalid free()" reports from valgrind, both with the
# stack free -> verto_cleanup (libverto.so) -> main (gssproxy), and
# "ERROR SUMMARY: 2 errors from 2 contexts".
set -euo pipefail
# Resolve this script's own directory so the gssproxy sources are found
# regardless of the caller's working directory (this is the repo root).
SCRIPT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
if ! command -v nix-shell >/dev/null 2>&1; then
echo "error: 'nix-shell' not found. Install Nix first: https://nixos.org/download" >&2
exit 127
fi
# nixpkgs pinned to the exact revision in flake.lock, fetched by commit so no
# channel or <nixpkgs> entry is needed. The sha256 is the flake's locked
# narHash (the unpacked-tree hash fetchTarball also uses), so this is fully
# reproducible and offline-cacheable.
NIXPKGS_REV="9ae611a455b90cf061d8f332b977e387bda8e1ca"
NIXPKGS_SHA="sha256-md8WlXOlfnIeHeOScMTTHFyf2d6iaTwPl2apR5EQ3P4="
# A shell environment providing valgrind plus a gssproxy built inline from THIS
# checkout's sources. The derivation is defined here (not imported from nix/),
# so the script works against any checkout -- including ones with no Nix infra.
# It mirrors the autotools build: autoreconf, configure (with a DocBook catalog
# that configure mandates for the man pages), make, then install just the daemon
# binary. krb5 pulls in its embedded BUILTIN_MODULE libverto, which is exactly
# the configuration that triggers the verto_cleanup() invalid free.
read -r -d '' NIX_EXPR <<EOF || true
let
pkgs = import (builtins.fetchTarball {
url = "https://github.com/NixOS/nixpkgs/archive/${NIXPKGS_REV}.tar.gz";
sha256 = "${NIXPKGS_SHA}";
}) { };
gssproxy = pkgs.stdenv.mkDerivation {
pname = "gssproxy-verto-repro";
version = "0";
src = pkgs.lib.cleanSource ${SCRIPT_DIR};
nativeBuildInputs = with pkgs; [
autoreconfHook
pkg-config
gettext
libxslt
libxml2
docbook-xsl-nons
docbook_xml_dtd_45
findutils
];
buildInputs = with pkgs; [
krb5
libverto
ding-libs
keyutils
popt
systemdLibs
];
configureFlags = [ "--with-initscript=none" "--with-selinux=no" ];
# configure (external/docbook.m4) requires xsltproc/xmllint/xmlcatalog and a
# catalog that resolves the DocBook manpages stylesheet, so build a combined
# one that delegates to the DTD and (non-namespaced) XSL catalogs offline.
preConfigure = ''
combinedCatalog="\$PWD/.nix-docbook-catalog.xml"
xmlcatalog --noout --create "\$combinedCatalog"
xmlcatalog --noout --add nextCatalog "" \\
"\${pkgs.docbook_xml_dtd_45}/xml/dtd/docbook/catalog.xml" "\$combinedCatalog"
xmlcatalog --noout --add nextCatalog "" \\
"\${pkgs.docbook-xsl-nons}/share/xml/docbook-xsl-nons/catalog.xml" "\$combinedCatalog"
configureFlagsArray+=("--with-xml-catalog-path=\$combinedCatalog")
'';
enableParallelBuilding = true;
doCheck = false;
# Keep symbols so valgrind prints a readable verto_cleanup/main backtrace.
dontStrip = true;
installPhase = ''
runHook preInstall
install -Dm755 gssproxy "\$out/bin/gssproxy"
runHook postInstall
'';
};
in
pkgs.mkShell { packages = [ pkgs.valgrind gssproxy ]; }
EOF
# The actual test, kept in its own file so the single quotes in the readiness
# string don't fight with the outer shell quoting.
INNER=$(mktemp)
trap 'rm -f "$INNER"' EXIT
cat >"$INNER" <<'INNER_EOF'
set -euo pipefail
D=$(mktemp -d)
trap 'rm -rf "$D"' EXIT
printf '[service/test]\n mechs = krb5\n euid = 0\n' >"$D/gp.conf"
valgrind \
--tool=memcheck \
--leak-check=no \
--num-callers=20 \
--error-exitcode=1 \
--log-file="$D/vg.log" \
gssproxy -i -s "$D/gp.sock" -c "$D/gp.conf" \
>"$D/out" 2>&1 &
PID=$!
# Wait up to 10s for the daemon's readiness line before signalling it.
for _ in $(seq 50); do
grep -q "Initialization complete" "$D/out" 2>/dev/null && break
sleep 0.2
done
kill -TERM "$PID" 2>/dev/null || true
wait "$PID" 2>/dev/null || true # valgrind exits non-zero once it finds errors
cat "$D/vg.log"
INNER_EOF
exec nix-shell --expr "$NIX_EXPR" --run "bash '$INNER'" |
|
None of the developers here uses nixos, sorry but this is not a useful and minimal reproducer. |
|
By the way, re-reading the valgrind trace in one of the comments I now know what's up, I'll submit a proper fix later. |
That is why I tried to make the script compatible with Debian and Fedora. Nix is a package manager that works on multiple operating systems. |
In the existing order there is a valgrind confirmed segfault when we free
record->filenameafter the module was closed.Will test this some more too!Tests pass locally.